Skip to content

ci: add one job branch protection can require - #90

Merged
srpatcha merged 3 commits into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate
Sep 8, 2026
Merged

srpatcha merged 3 commits into
embeddedos-org:masterfrom
Kartikey1306:ci/required-check-gate

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

required_status_checks is null on this repository's master — the same gap
eos#92 and ebuild#87 track. @srpatcha noted it applies here too:

The same gap is open in ebuild (#87 there) and eBoot — checks=null on all
three.

This repo has paid for it. #70 landed a TEST() macro that had lost a \ line
continuation; master was red from 08-31 08:07 until #77, and the 24 resulting
errors all pointed away from the cause. Nothing built the merge result before it
became master.

Why the existing checks cannot be required as they are

release is skipped on every pull request (if: startsWith(github.ref, 'refs/tags/v')), and a required check that is skipped never reports — the
pull request waits for a status that never arrives, which presents as
infrastructure flake rather than a policy.

Requiring test, build-arm and static-analysis individually works today and
stops working the next time a job is added, silently.

What this adds

One job, ci-gate, displayed as CI Gate — the single name to require:

  ci-gate:
    name: CI Gate
    needs: [test, build-arm, static-analysis]
    if: always()

The test is the part that keeps working

tests/unit/test_ci_gate.py (6 tests) parses ci.yml and fails if a job is
added without being wired into the gate.

Verified by mutation:

mutation result
drop build-arm from needs 2 failed
remove if: always() 1 failed
add a new job, forget to wire it in 2 failed
(restored) 6 passed

pyyaml is added to the pytest installer, which had only pytest pytest-cov.

Verification

item result
pytest tests/ PASS — 25 passed, 1 skipped
pytest tests/unit/test_ci_gate.py PASS — 6 passed
ctest PASS — 20/20
yaml.safe_load of the edited workflow PASS
mutation sweep above PASS — all three caught

Branches from master directly; unlike the other two repos, master here is
green.

What is still a maintainer action

Settings → Branches → master → Require status checks → add CI Gate. The
same PR is open on ebuild (#103) and eos (#121), so the name is identical in all
three.

@codecov-commenter

codecov-commenter commented Sep 1, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 83.92857% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tests/unit/test_ci_gate.py 83.92% 15 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

The gate was run against a real failure, not just reasoned about

Pushed a deliberately failing test to a branch on my fork so the whole workflow
would execute with one job red, and watched what the gate did.

Job results in that run:

  failure  Build & Test (Linux x86_64)      <- the deliberate failure
  success  Static Analysis (cppcheck + clang-tidy)
  skipped  Cross-compile ARM Cortex-M4      <- skipped, it needs: test
  skipped  Create GitHub Release            <- tag-only, correctly outside the gate
  failure  CI Gate                          <- reported red

And the gate's own log:

##[error]CI Gate failed. These jobs did not succeed:
  test: failure
  build-arm: skipped
##[error]Process completed with exit code 1.

Three things that confirms, which the workflow file alone does not:

  1. if: always() works. The gate ran at all despite an upstream failure. Had
    it been omitted, the gate would have been skipped — and a skipped required
    check never reports, so the pull request would have sat waiting for a status
    that never arrives instead of showing a red X.
  2. skipped is treated as a failure. build-arm was skipped and the gate
    named it. Had skipped counted as a pass, a job that verified nothing would
    have been reported as verified.
  3. release being outside the gate is correct. It was skipped and the gate
    did not care, which is exactly why it cannot be a required check itself.

The self-test branch has been deleted; nothing from it is in this PR.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBoot#90 "ci: add one job branch protection can require"

head: 8896d6a author: Kartikey1306 ci: pass

Verdict: The gate itself is correct and the evidence behind it is the strongest in this batch — a real run with a deliberately failing job, showing if: always() firing, skipped counted as a failure, and the gate reporting red, plus a three-way mutation sweep on the test. CI Gate reports pass in this PR's own checks.txt, so the job works. Two things need saying: the gate covers one of sixteen workflow files, not the repository, and the fail-open shape it fixes still exists in a sibling gate that is not tracked anywhere.

Findings

# Severity File:line Finding Recommended fix
1 Medium .github/workflows/ci.yml:180-217; tests/unit/test_ci_gate.py:82 CI Gate gates needs: [test, build-arm, static-analysis] — three jobs in ci.yml. The repository has 16 workflow files, and this PR's own checks.txt reports 26 checks across at least five separate runs: run …142934 is ci.yml (the three gated jobs plus release and the gate), but …142914 contributes Analyze (C/C++), …142986 contributes Cross-Platform ×3, EoSim ×11 and Simulation Gate, …143025 contributes Host Build & Tests, Cross-compile STM32F4 and a second Static Analysis, and CodeQL runs on its own. So the maintainer action the PR closes with — "Settings → Branches → master → Require status checks → add CI Gate" — would leave roughly 19 checks unrequired, including CodeQL, every EoSim platform leg, all three Cross-Platform legs and a second host build. master could still go red from any of them. The test has the same boundary: WORKFLOW = … / ".github" / "workflows" / "ci.yml" (tests/unit/test_ci_gate.py:82), so test_gate_covers_every_job_that_runs_on_a_pull_request protects ci.yml and nothing else — a job added to build.yml, codeql.yml or simulation-test.yml is not wired in and not noticed, which is precisely the silent-drift failure the PR is built to prevent. Say what the gate covers, and cover the rest. Concretely: (a) reword the body and the maintainer action to "require CI Gate and CodeQL, Simulation Gate, Host Build & Tests, Analyze (C/C++)" — or whichever set the maintainers judge required — so nobody reads one name as sufficient; (b) generalise the test to for wf in (Path(...) / ".github" / "workflows").glob("*.yml"), asserting that each workflow producing pull-request checks has a gate job whose needs covers every non-tag-only job in it. That turns a one-file guard into the repository-wide invariant the PR is arguing for, and it is the version that keeps working.
2 Medium .github/workflows/simulation-test.yml:93-109 The exact fail-open shape this PR removes from ci.yml is live in a sibling gate that the PR does not mention. Simulation Gate declares needs: [simulate, cross-platform] and then gates on one of them: it prints both results, tests only if [ "${{ needs.simulate.result }}" != "success" ], and otherwise prints ✅ All simulation checks passed. A red or skipped cross-platform — three OS legs — passes the gate. Simulation Gate reports pass in this PR's checks.txt and is a name a maintainer would plausibly require, so it is a required-check candidate that cannot fail for two thirds of what it claims to summarise. This PR's own words apply verbatim: "a job that did not run did not verify anything, and treating that as a pass is the fail-open shape #38, #59 and #82 removed from the boot path." The same pattern is in eosim-sanity.yml:135-156 (EoSim Sanity Gate, gating 1 of 5 dependencies) — that one is already recorded in .ai/autoreview/proposals/2026-09.md against #81, so I only note it; Simulation Gate appears in no proposal or issue I can find. Replace both gates' bodies with the toJSON(needs) + jq form this PR introduces, which is dependency-list-agnostic and cannot fall out of step with needs. It is a copy of the block at ci.yml:196-217. Doing it here would make this PR the single change that establishes one correct gate idiom repository-wide instead of adding a third idiom alongside two broken ones.
3 Low .github/workflows/ci.yml:39 This PR and #88 both rewrite the same pip3 install line — #90 adds pyyaml, #88 adds cryptography — so whichever merges second conflicts. Both additions are necessary and neither is a substitute. Related and worth noting for the maintainers: the 1 skipped in this PR's own verification table (pytest tests/ → "25 passed, 1 skipped") is the silently-skipped signing suite #88 diagnoses, so the two PRs are independent evidence of the same weakness in this installer line. Trivial to resolve; land as pip3 install pytest pytest-cov pyyaml cryptography. Flagging so it is not resolved by dropping one.

Verified clean, since a gate that fails open would be the serious defect here: the step cannot silently pass. GitHub's default shell for run: on Linux is bash -e {0}, so a jq failure inside bad=$(printf '%s' "$RESULTS" | jq -r …) fails the assignment and therefore the step, rather than leaving bad empty and falling through to echo "All jobs succeeded.". The jq filter selects on .value.result != "success", which catches failure, cancelled and skipped alike — matching the author's observed log, where build-arm: skipped was named as a reason for failure. test_ci_gate.py does a plain import yaml at module scope rather than pytest.importorskip, so a missing pyyaml is a collection error and a hard failure — the right choice, and notably stricter than the importorskip pattern used by the signing tests.

Architecture conformance

Conforms. §21 Infrastructure — ".github, website, docs, CI templates — Governance, release automation and documentation"; a workflow gate and its test belong exactly here. tests/unit/ is the right home for the test per .ai/architect.md's eBoot layout ("tests/ unit, functional, fuzz, performance, simulation"). Nothing in stage0/, stage1/, core/, hal/ or boards/ is touched, so §5.1's minimal-and-auditable TCB requirement is unaffected and no dependency points up a tier. §23's release model is respected: release is excluded from the gate because it is tag-gated, and test_jobs_left_out_of_the_gate_are_genuinely_tag_only enforces that the exclusion cannot be widened casually — that test is the best part of the PR, because it makes the exclusion a checked rule instead of a comment.

This is the enforcement side of a design gap already recorded. .ai/autoreview/proposals/2026-09.md carries "The evidence policy is silent on checks that verify nothing" (§28, triggered by eAI#39, eAI#41, eBoot#81), whose proposed §28.2 states: "An aggregating gate job must fail on any non-success among its dependencies, and must not print a summary asserting more than it checked." That is precisely what ci-gate implements and precisely what finding 2 shows Simulation Gate violates. No new proposal appended — this PR is evidence for the existing one, and I have added nothing to it because the text already covers the case.

Proposed changes

  1. Reword the body and the maintainer action to name the full required set rather than one name (finding 1a). Documentation only, and it is the difference between a maintainer closing the gap and believing they have.
  2. Generalise test_ci_gate.py to every workflow file (finding 1b).
  3. Port the toJSON(needs) gate body into simulation-test.yml:98-109 and eosim-sanity.yml:141-156 (finding 2), coordinating the latter with #81.
  4. Merge the pip3 install line with #88's rather than choosing between them (finding 3).

Items 1 and 4 are trivial. Item 2 is the one that makes the guarantee durable; item 3 is the one that makes it true today. None of them argues against merging the gate as it stands — it is a strict improvement on required_status_checks: null, and it should not wait on any of the above.

Not checked

  • Nothing was executed. No pytest, no ctest, no yaml.safe_load, no mutation sweep. Reproducing needs the PR head checked out, which the run brief forbids. The verification table — 25 passed, 1 skipped, 6 passed, ctest 20/20, and the three mutations each caught — is the author's and unverified by me. The gate's live behaviour is better evidenced than anything else in this batch: the 2026-09-01 comment shows a real run with a real failure and quotes the gate's own error output, and checks.txt here independently shows CI Gate pass 2s. I did not see that run and cannot confirm the log, but the two agree with each other and with the workflow source.
  • required_status_checks: null was not confirmed. That is the premise of the whole PR and needs gh api repos/embeddedos-org/eBoot/branches/master/protection, which I did not call. It rests on the author's statement and the quoted maintainer comment.
  • The check-to-workflow mapping in finding 1 is inferred from run IDs in checks.txt, not from reading all 16 workflow files. I read ci.yml, simulation-test.yml and eosim-sanity.yml. Which workflow produces Analyze (C/C++), Host Build & Tests, Cross-compile STM32F4 and the second Static Analysis I did not establish, only that their run IDs differ from ci.yml's. The count of ~19 uncovered checks follows from that grouping and should be confirmed before being quoted at a maintainer.
  • Finding 2 is from reading simulation-test.yml, not from a failing run. I did not construct a case where cross-platform fails and simulate succeeds to observe Simulation Gate pass anyway. The conditional is unambiguous on inspection, but the empirical form is what the author did for ci-gate and is what would settle it.
  • The other thirteen workflow files were not examined for gate jobs, fail-open conditionals, or jobs that ought to be required. nightly.yml, weekly.yml, scorecard.yml, build.yml, codeql.yml, book-build.yml, video-build.yml, deploy-pages.yml, cross-repo-dispatch.yml, release.yml, sync-release-branch.yml, auto-assign.yml and claude-code-review.yml are all unread. Finding 1b exists partly because I could not rule out more instances of finding 2 among them.
  • 24 of 26 checks pass; Create GitHub Release and assign report skipping, both expected on a PR and both correctly outside the gate. No required check failing — though as finding 1 notes, at this commit there are no required checks at all.
  • mergeStateStatus: BLOCKED, mergeable: MERGEABLE. No merge attempted; expect the ci.yml conflict with #88 described in finding 3.

Automated architecture review of 8896d6ade7eb — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

…rged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so
   embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.
…gate that failed open

Answers the review on embeddedos-org#90.

Finding 1 (Medium) -- `CI Gate` gates three jobs in ci.yml. This repository has
16 workflow files and a PR head reports 26 checks across five runs, so the
maintainer action in the body -- require one name -- would have left CodeQL,
every EoSim platform leg, all three Cross-Platform legs and a second host build
unrequired, and `master` could still go red from any of them. The rot-guard had
the same boundary: WORKFLOW was hardcoded to ci.yml, so a job added to
build.yml or codeql.yml was neither covered nor noticed.

Adds two tests over every workflow with a `pull_request` trigger:
REQUIRED_CHECKS names what a maintainer must actually require, NO_GATE excuses
the rest with a reason about the workflow itself, and a third test asserts each
gated workflow really has a job displaying under the name given.

Finding 2 (Medium) -- `Simulation Gate` had the exact fail-open shape this PR
removes from ci.yml: `needs: [simulate, cross-platform]`, then it printed both
results and branched on `simulate` alone before printing "All simulation checks
passed". A red or skipped `cross-platform` -- three OS legs -- passed it. Ported
the `toJSON(needs)` + `jq` body, which is dependency-list-agnostic and cannot
fall out of step with `needs:`.

And made it a rule rather than a one-off:
test_no_aggregating_gate_ignores_part_of_its_needs walks every gate in every
pull-request workflow and fails if a declared dependency is never compared.
Two refinements were needed to make it mean something:

  - printing a result is not testing it. The first version grepped for
    `needs.X.result` anywhere in the script, which Simulation Gate satisfied
    with its echo line. Only a line that compares counts.
  - book-build.yml's `summary` writes a step summary and claims no verdict.
    It is reporting, not gating, so the check applies only to jobs that either
    `exit 1` or assert that everything passed.

Finding 3 (Low) -- this branch had replaced master's
`pip3 install -r requirements.txt pytest-cov` with a hand-maintained list to
add pyyaml, which regressed the guard in tests/unit/test_requirements.py:

    these jobs run pytest over tests/ but never install from requirements.txt
    ... ['ci.yml:test']

Restored the requirements.txt install; pyyaml was already declared there, so
the hand list was not needed at all. That also removes the collision with embeddedos-org#88,
which edits the same line.

Verified:
  pytest tests/                          47 passed
  ctest                                  21/21 PASS
  yaml.safe_load of ci.yml, simulation-test.yml, book-build.yml   all parse
  discrimination, both ways:
    - reverting Simulation Gate to its fail-open body gives
        simulation-test.yml:sanity-gate declares needs
        ['simulate', 'cross-platform'] but never tests ['cross-platform']
    - with the toJSON(needs) body in place, 9 passed

Refs embeddedos-org#90
@Kartikey1306
Kartikey1306 force-pushed the ci/required-check-gate branch from 8896d6a to 1bd6a22 Compare September 3, 2026 10:43
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
… check

Answers the review on embeddedos-org#81. The install fix was right; the workflow it revives
could not have failed for most of what it says it verifies.

Finding 1 (Medium) -- the step named "Validate all platform configs" ran
`eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list`
prints "Available platforms (0)" without it and exits 0, so the step passed
having validated nothing -- and the same shape was in windows-sanity and
macos-sanity. All three now copy platforms/ from the pinned checkout and
assert a non-zero count via discover_platforms(), because the exit code is
exactly what cannot be trusted here.

Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no
--branch for its platforms/ copy, so the package came from
v${EOSIM_VERSION} while the platform data came from whatever the default
branch pointed at that morning. Now copies from the same pinned checkout; the
second clone is gone.

Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while
the tag and the package's declared version disagree upstream (v1.5.0 ships
"eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name
suggests, and nothing would have noticed if the tag moved. Now asserted. The
mismatch itself is EoSim's bug and is raised there rather than worked around
here.

Finding 4 (Medium) -- sanity-gate failed only on install-validate and then
printed "All EoSim sanity checks passed", which it would do with the other
four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which
cannot fall out of step with `needs:`. embeddedos-org#90 adds a test that enforces this
across every gate in the repository; this gate passes it.

Verified:
  yaml.safe_load of eosim-sanity.yml     parses, 6 jobs
  the gate now iterates toJSON(needs), no longer branches on
    install-validate alone, and no longer prints an "all passed" claim
  embeddedos-org#90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this
    workflow: eosim-sanity.yml is not among its offenders
  pytest tests/                          47 passed
  ctest                                  21/21 PASS

  NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch`
  only, so none of this PR's checks execute it -- which is finding 5, and it
  is the one piece of evidence this PR cannot produce from a fork branch
  without a maintainer dispatching it. The install sequence was verified
  locally end to end (clone -> pip install -> eosim --version 2.0.0 ->
  doctor -> run am62x --headless PASSED); the assertions added here are not
  covered by that and remain unexecuted.

Refs embeddedos-org#81
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
… check

Answers the review on embeddedos-org#81. The install fix was right; the workflow it revives
could not have failed for most of what it says it verifies.

Finding 1 (Medium) -- the step named "Validate all platform configs" ran
`eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list`
prints "Available platforms (0)" without it and exits 0, so the step passed
having validated nothing -- and the same shape was in windows-sanity and
macos-sanity. All three now copy platforms/ from the pinned checkout and
assert a non-zero count via discover_platforms(), because the exit code is
exactly what cannot be trusted here.

Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no
--branch for its platforms/ copy, so the package came from
v${EOSIM_VERSION} while the platform data came from whatever the default
branch pointed at that morning. Now copies from the same pinned checkout; the
second clone is gone.

Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while
the tag and the package's declared version disagree upstream (v1.5.0 ships
"eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name
suggests, and nothing would have noticed if the tag moved. Now asserted. The
mismatch itself is EoSim's bug and is raised there rather than worked around
here.

Finding 4 (Medium) -- sanity-gate failed only on install-validate and then
printed "All EoSim sanity checks passed", which it would do with the other
four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which
cannot fall out of step with `needs:`. embeddedos-org#90 adds a test that enforces this
across every gate in the repository; this gate passes it.

Verified:
  yaml.safe_load of eosim-sanity.yml     parses, 6 jobs
  the gate now iterates toJSON(needs), no longer branches on
    install-validate alone, and no longer prints an "all passed" claim
  embeddedos-org#90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this
    workflow: eosim-sanity.yml is not among its offenders
  pytest tests/                          38 passed
  ctest                                  21/21 PASS

  NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch`
  only, so none of this PR's checks execute it -- which is finding 5, and it
  is the one piece of evidence this PR cannot produce from a fork branch
  without a maintainer dispatching it. The install sequence was verified
  locally end to end (clone -> pip install -> eosim --version 2.0.0 ->
  doctor -> run am62x --headless PASSED); the assertions added here are not
  covered by that and remain unexecuted.

Refs embeddedos-org#81
Finishes finding 3 from the review on embeddedos-org#90. Findings 1 and 2 were already
addressed in 1bd6a22; this is the one left.

test_gate_fails_on_any_non_success_result searched the gate's `run:` text for
`!= "success"` and `exit 1`. That is a string match on an implementation, not a
check of behaviour: it passes for those tokens sitting in a comment or an
unreachable branch, and fails for a correct rewrite expressing the same rule
differently. The only real evidence that the rule works was the fork
experiment, which is a one-off nothing re-runs.

The rule now lives in .github/scripts/ci-gate-check.sh, and pytest runs it
against real inputs: success, failure, skipped, cancelled, a mixed set, and an
empty context. Both gates -- ci.yml's `CI Gate` and simulation-test.yml's
`Simulation Gate` -- call that one script, and a further test asserts they do,
so the behavioural cases cover every gate rather than one.

The script also refuses an empty or null `needs` context. No results is not the
same as no failures, and a gate that passes when it was handed nothing is the
same fail-open shape in a different place.

Verified by mutation: weakening the rule to `== "failure"` fails 3 of 18, and a
gate that stops calling the script fails 1 of 18. Restored, 18 pass.

42 Python tests pass (1 skipped), ctest 20/20, all 16 workflow files parse.
@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Finding 3 addressed at ffb5ff9. Findings 1 and 2 were already done in
1bd6a22, so this is the remainder.

The rule is now executed, not pattern-matched. It lives in
.github/scripts/ci-gate-check.sh, and pytest runs it against real inputs:

input exit
{"a":{"result":"success"}} 0
{"a":{"result":"failure"}} 1
{"a":{"result":"skipped"}} 1
{"a":{"result":"cancelled"}} 1
{"a":{"result":"success"},"b":{"result":"skipped"}} 1
"" / null 1

Both gates — CI Gate and Simulation Gate — call that one script, and a
further test asserts they do, so the behavioural cases cover every gate rather
than one. Your point that the fork experiment was a one-off nothing re-runs was
the right objection; it is now six parametrized cases on every push.

The last row is an addition beyond the finding: the script refuses an empty or
null context. No results is not the same as no failures, and a gate that
passes when handed nothing is the same fail-open shape one level up.

Verified by mutation: weakening the rule to == "failure" fails 3 of 18; a
gate that stops calling the script fails 1 of 18. Restored, 18 pass. Full run:
42 Python tests (1 skipped), ctest 20/20, all 16 workflow files parse.

On finding 2, and a sibling in the other repo

Worth reporting back because your finding generalised. The same shape existed
in eos, in a place neither of us had looked: eos-simulation.yml's
Full-stack integration summary declares

needs: [build-kernel, simulate-qemu, test-middleware, test-cad-pipeline]
if: always()

prints PASS/FAIL for all four — and its only exit 1 tested
needs.simulate-qemu.result. Three of its four dependencies could fail while
it reported success, and it is the most requirable-looking name in that
repository. Fixed in eos#121 with the same script.

So that is three instances of one pattern across two repos: Simulation Gate,
EoSim Sanity Gate, and Full-stack integration summary. The proposed §28.2
text you quoted — "must fail on any non-success among its dependencies, and
must not print a summary asserting more than it checked" — describes all three
exactly. A shared script is what makes that enforceable rather than repeatedly
re-litigated, which is why I extracted one rather than inlining the block a
third time.

Finding 3's conflict note

Agreed, and it resolves as you said — pip3 install -r requirements.txt pytest-cov on this branch already carries pyyaml via requirements.txt, so
#88's cryptography addition and this do not collide the way the review
predicted. Flagging in case that changes: the resolution is to union the two,
never to pick one.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBoot#90 "ci: add one job branch protection can require"

head: ffb5ff9 author: Kartikey1306 ci: pass

Verdict: The gate is correct — if: always(), non-success of any kind fails, release
correctly outside it, and the rule in an executable script rather than grepped out of YAML.
I ran the script against every result shape and it behaves as documented, except on an empty
dependency set. The bigger problem is that the PR body and tests/unit/test_ci_gate.py
disagree about the deliverable, and the tests are right: CI Gate covers ci.yml and
nothing else, so the maintainer action in the body leaves most of eBoot's pull-request
surface unrequired.

Findings

# Severity File:line Finding Recommended fix
1 Medium pr body ("What is still a maintainer action"); tests/unit/test_ci_gate.py:26-35, 159-176 The body says ci-gate is "the single name to require" and the maintainer action is "add CI Gate". The PR's own test file says otherwise, in as many words: "CI Gate covers ci.yml and nothing else -- cross-workflow needs is not something GitHub offers -- so the required set is this mapping, not one name", and test_every_pull_request_workflow_is_accounted_for documents that requiring one name "leaves every other workflow's checks unrequired". Enumerated the workflows: on pull_request eBoot also runs build.yml (Host Build & Tests, Cross-compile STM32F4, Static Analysis), codeql.yml (Analyze (C/C++)), simulation-test.yml (Simulation Gate) and book-build.yml. A maintainer who follows the body requires one check and believes the gap is closed. Replace the maintainer action with the actual list: CI Gate, CodeQL, Simulation Gate (requirable once this PR lands — see finding 2), and either build.yml's three names or a gate for it (finding 3). The test file's REQUIRED_CHECKS mapping is the right shape; make the body quote it instead of contradicting it.
2 Medium tests/unit/test_ci_gate.py:51-57 simulation-test.yml sits in NO_GATE because its gate "tests only needs.simulate.result and then prints 'All simulation checks passed'… Requiring it today would assert more than it checks -- fixed below rather than excused". This PR is the fix: simulation-test.yml:95-109 now pipes toJSON(needs) through the shared script. So the one workflow this PR made trustworthy is still the one it excuses, and Simulation Gate stays unrequired. Move "simulation-test.yml": "Simulation Gate" into REQUIRED_CHECKS and delete the NO_GATE entry. test_gated_workflows_really_have_their_gate will then check it, and the entry becomes part of the maintainer action in finding 1.
3 Medium tests/unit/test_ci_gate.py:47-50 build.yml is excused because its jobs are "under stable names that can be required directly; wrapping them adds a layer without adding coverage." That is precisely the argument the PR body rejects two paragraphs earlier for ci.yml: "Requiring test, build-arm and static-analysis individually works today and stops working the next time a job is added, silently." build.yml has three such jobs on every push and pull request — a second host build and a second static analysis — and no gate and no rot-guard. The reasoning cannot be sound in one file and unsound in the other. Give build.yml the same three-line gate reusing .github/scripts/ci-gate-check.sh, and list it in REQUIRED_CHECKS. If the real reason to leave it alone is that build.yml and ci.yml overlap and one should go, say that instead — it is a better argument, and it is a maintainer decision worth an issue.
4 Medium .github/scripts/ci-gate-check.sh:19-36 The gate passes on an empty dependency set. Verified: printf '{}' | .github/scripts/ci-gate-check.sh → rc=0, "All jobs succeeded." The guard at :19 catches "" and "null" only; jq 'to_entries[]' over {} yields nothing, bad is empty, and the script reports success. The header says "a job that did not run did not verify anything" — zero jobs is the limit case of that, and it is treated as a pass. A needs: key that is deleted, mistyped, or narrowed to [] turns the required check green having verified nothing: the fail-open shape the script exists to remove. test_gate_script_refuses_an_empty_context parametrizes "" and null but not {}. count=$(printf '%s' "$results" | jq 'length'); [ "$count" -gt 0 ] || { echo "::error::CI Gate had no dependencies; refusing to pass."; exit 1; } before the bad computation, and add "{}" to that test's parameters.
5 Medium .github/workflows/ci.yml:30-40 Seven lines of comment are deleted and the pip3 install line they explain is unchanged, byte for byte. The deleted text explained why the job installs -r requirements.txt rather than a hand-maintained list, and why pytest-cov is kept separate — that is the eBoot#88 silent-skip story, still true and still the reason the line looks the way it does. The body justifies the edit as "pyyaml is added to the pytest installer, which had only pytest pytest-cov"; nothing was added — requirements.txt:1 already declares pyyaml>=6.0 and master already installs -r requirements.txt pytest-cov. .ai/reviewer.md asks whether anything was deleted or overwritten without explanation. Restore the deleted comment and append the pyyaml line to it rather than in place of it. The install line needs no change.
6 Low pr body ("Verification") "Branches from master directly; unlike the other two repos, master here is green." origin/master@22d8f8b does not compile — duplicate point_is_identity in core/ed25519_verify.c:281,338 and include/eos_image.h:135,142 asserting on a reserved member that #93 removed. gh run list --branch master shows CI — eBoot, eBoot Build & Test and CodeQL all failure on that commit. This PR's own diff carries the fix for both, which is why it builds. The claim is the strongest argument for the PR, stated backwards: master is red right now, and a required check is what would have stopped it. Say that. "master is red today for the same reason #70 made it red — nothing built the merge result" is a better opening than the incident from 08-31.
7 Low pr body ("The test is the part that keeps working", "Verification") "tests/unit/test_ci_gate.py (6 tests)" and "PASS — 6 passed". The file defines 12 test functions, two of them parametrized. "ctest PASS — 20/20" does not match either: the head's suite is 21 tests (measured), and it cannot have been 20 on master, which does not build. Re-measure and quote the current numbers. The file is better than the body claims, which is an odd way to undersell it.
8 Low tests/unit/test_ci_gate.py:99-103 _only_runs_on_tags returns true for any if containing the substring refs/tags, so a negated or unrelated condition mentioning tags would count as tag-only and let a job out of the gate. It is the check that decides which jobs may be excluded, so it is worth more than a substring. Match the shape the workflow actually uses: re.search(r"startsWith\(\s*github\.ref\s*,\s*'refs/tags", condition), and reject conditions containing !.

Architecture conformance

Master design §28 (status/evidence policy — a claim needs the evidence its state requires),
§23 (release model: master is the line of development, every PR merges there), §21
Infrastructure tier (".github, website, docs, CI templates — governance, release automation");
.github/STANDARDS.md "Release model" (master is written by maintainers via PRs).
Conforms. This is Infrastructure-tier work on Infrastructure-tier files and introduces no
dependency in either direction. Nothing in the diff imports, includes or links anything.

The design mandates the evidence model but never says what enforces it at merge time. §23.1
and §28 describe channels and claim states; .github/STANDARDS.md describes branch roles.
Neither requires a status check on master, which is why required_status_checks: null is
not a policy violation today — it is a gap. That is worth a design proposal rather than a
finding against this PR, and I have filed one.

Verified by running, on master + this patch:

.github/scripts/ci-gate-check.sh, stdin -> rc:
  {"test":{"result":"success"},"build-arm":{"result":"success"}}   -> 0  "All jobs succeeded."
  {"test":{"result":"success"},"build-arm":{"result":"skipped"}}   -> 1  "  build-arm: skipped"
  {"test":{"result":"failure"}}                                    -> 1  "  test: failure"
  {}                                                               -> 0  finding 4
  null                                                             -> 1
  (empty)                                                          -> 1

yaml.safe_load of both edited workflows                            -> OK
ci.yml jobs: test, build-arm, static-analysis, release, ci-gate
  ci-gate  name='CI Gate'  if='always()'  needs=[test, build-arm, static-analysis]
  release  if="startsWith(github.ref, 'refs/tags/v')"   -> genuinely tag-only
  -> the gate covers every non-tag job in ci.yml. Complete for this workflow.

cmake --build build/host && ctest --no-tests=error -j4  -> 21/21 passed

The design decisions are right and worth keeping on the record: if: always() so a failure
is a red X rather than a pull request waiting on a status that never arrives; skipped
treated as failure; release outside the gate because a required check that never reports
blocks merges forever; and the rule in a script so test_gate_script_accepts_only_all_success
executes it instead of pattern-matching the YAML. test_no_aggregating_gate_ignores_part_of_ its_needs is the strongest thing here — it catches the "prints one result, branches on
another" shape generically, across every pull-request workflow, and it is what found the
Simulation Gate defect this PR also fixes.

Proposed changes

  1. Rewrite the maintainer action as the real required set; promote simulation-test.yml into
    REQUIRED_CHECKS (findings 1, 2).
  2. Gate build.yml, or replace its NO_GATE reason with the honest one (finding 3).
  3. Reject an empty dependency set in the script, and parametrize {} (finding 4).
  4. Restore the deleted ci.yml comment and append to it (finding 5).
  5. Correct the three measurements in the body (findings 6, 7).
  6. Tighten _only_runs_on_tags (finding 8).

Not checked

  • pytest is not installed in this environment, so none of test_ci_gate.py was run.
    Every claim I make about that file is from reading it; the script behaviour in the table
    above was executed directly, not through pytest. The body's mutation sweep ("drop
    build-arm from needs → 2 failed", etc.) was not reproduced.
  • Branch protection itself was not inspected. required_status_checks: null on master is
    taken from the PR body and the cross-references to eos#92 / ebuild#87; I did not query the
    branch-protection API, and this run has no authority to change it. Whether CI Gate is
    actually addable under the current ruleset is unverified.
  • The gate was not observed running on GitHub. checks.txt for this head lists CI Gate
    among the passing checks, which is evidence the job runs and succeeds on a green branch —
    not evidence it fails correctly on a red one.
  • book-build.yml's exclusion ("a docs failure should not block a code merge") is a
    maintainer policy call, not a technical one, and I have not second-guessed it.
  • mergeStateStatus: BLOCKED, reviewDecision: REVIEW_REQUIRED — not investigated.
  • This PR also carries the core/ed25519_verify.c, include/eos_image.h and
    tests/unit/test_ed25519.c changes that repair master's broken host build, identical to
    eBoot#81, #88 and #89. Reviewed under #81; not re-litigated here.

Automated architecture review of ffb5ff90bf14 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
…harnesses

Answers the second review on embeddedos-org#84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: embeddedos-org#90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of embeddedos-org#84 and embeddedos-org#90 lands second
  must add fuzz-build to that list -- and embeddedos-org#90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries embeddedos-org#94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs embeddedos-org#84
srpatcha added a commit that referenced this pull request Sep 8, 2026
* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fdt): bound a device tree parser that was never compiled

core/fdt_loader.c is in no source list, so it has never been built. The
header offers it to callers, rtos_boot.c is named as the consumer, and
nothing catches what is in it -- not the compiler, not ctest, not the
sanitizer job.

What is in it is a parser for a blob that comes out of flash, whose every
header field it uses as an offset or a length without checking any of
them. eos_fdt_validate() looks at magic and version only, so it accepts
a blob whose off_dt_struct points anywhere:

    hdr.totalsize     = 40      (the header alone)
    hdr.off_dt_struct = 0x100000
    eos_fdt_validate(blob) -> 0
    eos_fdt_get_prop(...)  -> AddressSanitizer: BUS, READ at
                              fdt_loader.c:25 in fdt_read_u32

Five more, all reachable the same way:

* the tag read at the top of the loop takes 4 bytes where the loop
  condition guarantees 1;
* strlen() on a node name reads until it finds a zero, which for a name
  running to the end of the block is past it;
* nameoff indexes the strings block unchecked, so strcmp() reads from an
  arbitrary address;
* a property len is clamped to the caller's buffer before the memcpy,
  which bounds the write but not the read -- an oversized len copies
  whatever follows the blob out to the caller;
* FDT_END_NODE decrements depth with no floor.

Bound them. validate() is the gate every path goes through, so the block
offsets are checked against totalsize there, and get_prop() calls it
before trusting the header. Inside the loop each read is checked for the
width it takes, the name and property-name scans are bounded by memchr
within their blocks, and the padded advances are re-checked for overrun.

Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest:
ten cases, one well-formed tree that must still parse and nine malformed
ones. Against the unfixed parser the suite fails on the third; with the
bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE.

Not fixed here, because it is a behaviour change rather than a safety
one: node paths below the root do not resolve. _get_prop derives the
depth to match from a slash count, so "/chosen" looks for depth 1, which
is the root -- "chosen" is at depth 2. Only "/" resolves today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fdt): bound the parser against the caller's length, not the blob's claim

Follow-up to the review on #84. Three findings from it, all in files this
branch already owns.

Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative
to hdr->totalsize, which is read out of the same untrusted blob. Checking the
block offsets against it proves the header is internally consistent and says
nothing about how many bytes are really mapped: a 40-byte buffer declaring
totalsize = 0x100000 with agreeing offsets passed every check, and
eos_fdt_get_prop() then walked a megabyte past the allocation. The original
reproducer (an inflated *offset* against an honest totalsize) was rejected;
its mirror image was not.

Adds the length-carrying entry points eos_fdt_validate_sized() and
eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and
check totalsize against that before anything else. The existing two-argument
forms stay as wrappers that trust the blob's own totalsize, documented in the
header as a warrant the caller has to make good; eos_fdt_load() now passes
max_size, which it had all along.

Finding 2 (Medium) -- a property larger than the caller's buffer was copied
short and reported as success, so a clipped value was indistinguishable from
a complete one. In a boot path this reads bootargs. Now returns -7 and sets
*buf_len to the full length so the caller can size a retry.

Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing
harnesses. .ai/security.md names device tree among the parsers that get fuzz
coverage rather than unit tests alone. It drives the sized entry points and
passes the real size, which is what makes the harness meaningful -- the
unsized forms would let a fuzzer authorise its own out-of-bounds read.

Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check
in eos_fdt_load(); validate() already rejects that blob, and the comment
above it described a bound it was not applying.

Header now documents the full -1..-8 return code table.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON, ctest              21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  14 tests PASS (was 10)
  new tests against the pre-fix parser             both FAIL as they should
    - inflated totalsize accepted at the sized entry point
    - truncating call returned 0 instead of -7
  grep for eos_fdt_get_prop / eos_fdt_validate     no callers outside the
    parser and its tests, so the -7 change breaks nothing today

Refs #84

* fix(build): drop the duplicate core/fdt_loader.c registration

master gained core/fdt_loader.c in eboot_core while this branch was open, so
after the rebase it appeared twice and
tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once
failed:

    AssertionError: ['core/fdt_loader.c'] is not false :
    duplicate eboot_core sources: ['core/fdt_loader.c']

Kept master's entry, dropped the one this branch added. That guard is the
same shape as the one this stack adds for toolchain specs -- a build-file
check that catches the class rather than the instance -- and it did its job.

Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped.

Refs #84

* fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses

Answers the second review on #84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: #90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second
  must add fuzz-build to that list -- and #90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries #94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs #84

* ci: move the fuzz-harness build to its own PR

The fuzz-build job added in the previous commit does its job -- it is red, on
harnesses this PR did not write:

    undefined reference to `eos_fw_update_init'
    undefined reference to `eos_fw_update_process_chunk'
    undefined reference to `eos_recovery_parse_packet'

None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and
fuzz_recovery_protocol.c declare an API that was never written, exactly as
eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever
because nothing ever linked them.

That is a real finding and it deserves a fix, not a red tick on an unrelated
PR. The job and the repairs move to their own change; this PR keeps
tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for.

Refs #84

* fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage

Answers the third review on #84.

Finding 3 (Low, and the one that matters on hardware) -- the header was read
through a cast fdt_header_t* and direct member loads, while every struct-block
read goes through the fdt_read_u32() memcpy helper precisely because the blob
may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an
odd offset: nothing promises 4-byte alignment, and on a strict-alignment
cross target a direct member load is the same fault class this parser exists
to avoid. The unit suite could never catch it -- blob_t.bytes happens to be
aligned.

All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule
for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains
`undefined` so the fuzz job checks alignment too.

Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the
walk skipped 4 bytes and treated whatever followed as the next token, so a
struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a
TCB parser that walks garbage to completion is accepting input it does not
understand. FDT_NOP -- the one legal unknown, padding the spec allows between
tokens -- is now named in the header and passes; anything else returns -6.

Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101
(nothing added here; the harness is inert until that job lands and then
compiled by it -- said on the thread, not just here), and the header no longer
documents return code -8, which nothing at this head returns. #85 adds the -8
return and re-documents it together with the FDT_MAX_PATH_DEPTH move.

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          16 tests PASS (was 13)
  discrimination, each fix in isolation:
    default: break restored     -> test_a_garbage_tag_is_refused_not_skipped
                                   FAILS (expects -6, gets "not found")
    direct member load restored -> under UBSan the new unaligned-blob test
                                   reports "load of misaligned address
                                   0x...671 for type 'const uint32_t'" at the
                                   exact line -- and cannot fire on the fixed
                                   code, which runs the same test clean
  NOP counter-check: interleaved FDT_NOP tokens still resolve, so the
  stricter default does not reject real trees.

Refs #84

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
srpatcha pushed a commit that referenced this pull request Sep 8, 2026
…ped (#81)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* ci: make the EoSim workflow able to fail for the reasons it claims to check

Answers the review on #81. The install fix was right; the workflow it revives
could not have failed for most of what it says it verifies.

Finding 1 (Medium) -- the step named "Validate all platform configs" ran
`eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list`
prints "Available platforms (0)" without it and exits 0, so the step passed
having validated nothing -- and the same shape was in windows-sanity and
macos-sanity. All three now copy platforms/ from the pinned checkout and
assert a non-zero count via discover_platforms(), because the exit code is
exactly what cannot be trusted here.

Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no
--branch for its platforms/ copy, so the package came from
v${EOSIM_VERSION} while the platform data came from whatever the default
branch pointed at that morning. Now copies from the same pinned checkout; the
second clone is gone.

Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while
the tag and the package's declared version disagree upstream (v1.5.0 ships
"eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name
suggests, and nothing would have noticed if the tag moved. Now asserted. The
mismatch itself is EoSim's bug and is raised there rather than worked around
here.

Finding 4 (Medium) -- sanity-gate failed only on install-validate and then
printed "All EoSim sanity checks passed", which it would do with the other
four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which
cannot fall out of step with `needs:`. #90 adds a test that enforces this
across every gate in the repository; this gate passes it.

Verified:
  yaml.safe_load of eosim-sanity.yml     parses, 6 jobs
  the gate now iterates toJSON(needs), no longer branches on
    install-validate alone, and no longer prints an "all passed" claim
  #90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this
    workflow: eosim-sanity.yml is not among its offenders
  pytest tests/                          38 passed
  ctest                                  21/21 PASS

  NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch`
  only, so none of this PR's checks execute it -- which is finding 5, and it
  is the one piece of evidence this PR cannot produce from a fork branch
  without a maintainer dispatching it. The install sequence was verified
  locally end to end (clone -> pip install -> eosim --version 2.0.0 ->
  doctor -> run am62x --headless PASSED); the assertions added here are not
  covered by that and remain unexecuted.

Refs #81
@srpatcha
srpatcha merged commit 7958805 into embeddedos-org:master Sep 8, 2026
27 checks passed
srpatcha added a commit that referenced this pull request Sep 8, 2026
…85)

* fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken

master (22d8f8b) does not compile. Two independent double-merges, both the
same shape: two PRs fixing adjacent things landed on stale bases, each was
green on its own branch, and the result was never rebuilt.

1. include/eos_image.h — #93 replaced reserved[30] with tlv_len (2) +
   tlv_hash[28], preserving every offset. #87 merged afterwards carrying
   asserts written against the older struct:

     error: no member named 'reserved' in 'eos_image_header_t'   (x2)

   #93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert
   was a duplicate; the width assert had no replacement and is restored as two
   asserts covering both halves of the same 30-byte span. No offset moves and
   the wire format is unchanged.

2. core/ed25519_verify.c — #86 and #57 both landed a subgroup guard, so the
   file carried two byte-identical point_is_identity() definitions:

     error: redefinition of 'point_is_identity'

   Only #57's public_key_is_valid_subgroup() is wired to the call site, so
   #86's key_has_prime_order() was dead. Kept the live function, folded #86's
   fuller rationale onto it, deleted the duplicate.

3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of
   test_ed25519_identity_key_forgery_rejected, main() calling it twice and two
   tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery
   referencing k_low_order[] and messages[] that the merge had dropped.

   While restoring the corpus, corrected it (review finding on #86): the array
   claimed to hold "the eight low-order point encodings" and held five. Every
   order here was computed rather than copied — decode y, recover x, add the
   point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8,
   8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8
   encodings. D9FF..FF was in the array and is not a low-order point at all —
   no x satisfies the curve equation for that y — so it moves to a separate
   k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1).

   tests_run was assigned a literal (11) in main() and never incremented,
   which is how the duplicate call and the two unregistered tests went
   unnoticed. The TEST macro now increments it, so the total cannot drift.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON on master   FAILS to build, 3 errors
  same with this commit                    builds clean
  ctest                                    21/21 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   21/21 PASS
  pytest tests/                            24 passed, 1 skipped
  test_ed25519                             14/14 PASS (was 11 claimed, 12 run)
  discrimination, with `public_key_is_valid_subgroup` disabled:
    test_ed25519_low_order_keys_rejected   FAILS, as it must
    test_ed25519_non_canonical_...         still PASSES — those are refused by
      unpackneg() on canonicality, a different mechanism, which is the reason
      they are held in a separate array rather than counted among the eight.

* fix(fdt): bound a device tree parser that was never compiled

core/fdt_loader.c is in no source list, so it has never been built. The
header offers it to callers, rtos_boot.c is named as the consumer, and
nothing catches what is in it -- not the compiler, not ctest, not the
sanitizer job.

What is in it is a parser for a blob that comes out of flash, whose every
header field it uses as an offset or a length without checking any of
them. eos_fdt_validate() looks at magic and version only, so it accepts
a blob whose off_dt_struct points anywhere:

    hdr.totalsize     = 40      (the header alone)
    hdr.off_dt_struct = 0x100000
    eos_fdt_validate(blob) -> 0
    eos_fdt_get_prop(...)  -> AddressSanitizer: BUS, READ at
                              fdt_loader.c:25 in fdt_read_u32

Five more, all reachable the same way:

* the tag read at the top of the loop takes 4 bytes where the loop
  condition guarantees 1;
* strlen() on a node name reads until it finds a zero, which for a name
  running to the end of the block is past it;
* nameoff indexes the strings block unchecked, so strcmp() reads from an
  arbitrary address;
* a property len is clamped to the caller's buffer before the memcpy,
  which bounds the write but not the read -- an oversized len copies
  whatever follows the blob out to the caller;
* FDT_END_NODE decrements depth with no floor.

Bound them. validate() is the gate every path goes through, so the block
offsets are checked against totalsize there, and get_prop() calls it
before trusting the header. Inside the loop each read is checked for the
width it takes, the name and property-name scans are bounded by memchr
within their blocks, and the padded advances are re-checked for overrun.

Adds the file to eboot_core and tests/unit/test_fdt_loader.c to ctest:
ten cases, one well-formed tree that must still parse and nine malformed
ones. Against the unfixed parser the suite fails on the third; with the
bounds in place the whole suite is 20/20, and 20/20 under EBLDR_SANITIZE.

Not fixed here, because it is a behaviour change rather than a safety
one: node paths below the root do not resolve. _get_prop derives the
depth to match from a slash count, so "/chosen" looks for depth 1, which
is the root -- "chosen" is at depth 2. Only "/" resolves today.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(fdt): bound the parser against the caller's length, not the blob's claim

Follow-up to the review on #84. Three findings from it, all in files this
branch already owns.

Finding 1 (High) -- every bound in eos_fdt_validate() was expressed relative
to hdr->totalsize, which is read out of the same untrusted blob. Checking the
block offsets against it proves the header is internally consistent and says
nothing about how many bytes are really mapped: a 40-byte buffer declaring
totalsize = 0x100000 with agreeing offsets passed every check, and
eos_fdt_get_prop() then walked a megabyte past the allocation. The original
reproducer (an inflated *offset* against an honest totalsize) was rejected;
its mirror image was not.

Adds the length-carrying entry points eos_fdt_validate_sized() and
eos_fdt_get_prop_sized(), which take the bytes the caller actually owns and
check totalsize against that before anything else. The existing two-argument
forms stay as wrappers that trust the blob's own totalsize, documented in the
header as a warrant the caller has to make good; eos_fdt_load() now passes
max_size, which it had all along.

Finding 2 (Medium) -- a property larger than the caller's buffer was copied
short and reported as success, so a clipped value was indistinguishable from
a complete one. In a boot path this reads bootargs. Now returns -7 and sets
*buf_len to the full length so the caller can size a retry.

Finding 3 (Medium) -- adds tests/fuzz/fuzz_fdt.c alongside the five existing
harnesses. .ai/security.md names device tree among the parsers that get fuzz
coverage rather than unit tests alone. It drives the sized entry points and
passes the real size, which is what makes the harness meaningful -- the
unsized forms would let a fuzzer authorise its own out-of-bounds read.

Finding 6 (Low) -- drops the unreachable `total < sizeof(fdt_header_t)` check
in eos_fdt_load(); validate() already rejects that blob, and the comment
above it described a bound it was not applying.

Header now documents the full -1..-8 return code table.

Verified:
  cmake -DEBLDR_BUILD_TESTS=ON, ctest              21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  14 tests PASS (was 10)
  new tests against the pre-fix parser             both FAIL as they should
    - inflated totalsize accepted at the sized entry point
    - truncating call returned 0 instead of -7
  grep for eos_fdt_get_prop / eos_fdt_validate     no callers outside the
    parser and its tests, so the -7 change breaks nothing today

Refs #84

* fix(build): drop the duplicate core/fdt_loader.c registration

master gained core/fdt_loader.c in eboot_core while this branch was open, so
after the rebase it appeared twice and
tests/unit/test_cmake_core_sources.py::test_core_sources_are_registered_once
failed:

    AssertionError: ['core/fdt_loader.c'] is not false :
    duplicate eboot_core sources: ['core/fdt_loader.c']

Kept master's entry, dropped the one this branch added. That guard is the
same shape as the one this stack adds for toolchain specs -- a build-file
check that catches the class rather than the instance -- and it did its job.

Verified: ctest 22/22 PASS, pytest 24 passed 1 skipped.

Refs #84

* fix(fdt): one length-carrying API, and a CI job that builds the fuzz harnesses

Answers the second review on #84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: #90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of #84 and #90 lands second
  must add fuzz-build to that list -- and #90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries #94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs #84

* ci: move the fuzz-harness build to its own PR

The fuzz-build job added in the previous commit does its job -- it is red, on
harnesses this PR did not write:

    undefined reference to `eos_fw_update_init'
    undefined reference to `eos_fw_update_process_chunk'
    undefined reference to `eos_recovery_parse_packet'

None of those exist anywhere in the tree. tests/fuzz/fuzz_fw_update.c and
fuzz_recovery_protocol.c declare an API that was never written, exactly as
eos#50's fuzz_devicetree declared eos_dtb_parse(). They have compiled forever
because nothing ever linked them.

That is a real finding and it deserves a fix, not a red tick on an unrelated
PR. The job and the repairs move to their own change; this PR keeps
tests/fuzz/fuzz_fdt.c, which is the harness it is responsible for.

Refs #84

* fix(fdt): read the header the way the struct block is read, and stop resynchronising on garbage

Answers the third review on #84.

Finding 3 (Low, and the one that matters on hardware) -- the header was read
through a cast fdt_header_t* and direct member loads, while every struct-block
read goes through the fdt_read_u32() memcpy helper precisely because the blob
may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an
odd offset: nothing promises 4-byte alignment, and on a strict-alignment
cross target a direct member load is the same fault class this parser exists
to avoid. The unit suite could never catch it -- blob_t.bytes happens to be
aligned.

All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule
for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains
`undefined` so the fuzz job checks alignment too.

Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the
walk skipped 4 bytes and treated whatever followed as the next token, so a
struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a
TCB parser that walks garbage to completion is accepting input it does not
understand. FDT_NOP -- the one legal unknown, padding the spec allows between
tokens -- is now named in the header and passes; anything else returns -6.

Findings 1, 2 and the -8 line: the fuzz-build job's single home is #101
(nothing added here; the harness is inert until that job lands and then
compiled by it -- said on the thread, not just here), and the header no longer
documents return code -8, which nothing at this head returns. #85 adds the -8
return and re-documents it together with the FDT_MAX_PATH_DEPTH move.

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          16 tests PASS (was 13)
  discrimination, each fix in isolation:
    default: break restored     -> test_a_garbage_tag_is_refused_not_skipped
                                   FAILS (expects -6, gets "not found")
    direct member load restored -> under UBSan the new unaligned-blob test
                                   reports "load of misaligned address
                                   0x...671 for type 'const uint32_t'" at the
                                   exact line -- and cannot fire on the fixed
                                   code, which runs the same test clean
  NOP counter-check: interleaved FDT_NOP tokens still resolve, so the
  stricter default does not reject real trees.

Refs #84

* fix(fdt): resolve a node by its path, not by its depth and last name

Replaces slash-counting and last-component matching with a component walk
gated on ancestor matches, so /soc/uart and /decoy/uart are distinguishable.

Also carries the fixes from the review on this PR:

Finding 1 (High) -- the property arm tested in_target alone, and in_target is
cleared only by the target's own FDT_END_NODE, so it stayed true for the
entire subtree and a property found on a *child* was returned as the target's
own. On this PR's own decoy tree, eos_fdt_get_prop(blob, "/soc", "reg", ...)
returned 0 with "soc-uart" when /soc has no reg property at all and the
correct answer is -5. Real device trees emit properties before subnodes, so a
property that is present on the target is still found first -- the bug bites
when the target lacks it, turning "not found" into a silently wrong value
from a nested node. For an attacker-supplied blob the ordering is not
enforced at all. Fixed by also requiring depth == target_depth, which is
exact: depth is target_depth for the target's own properties and
target_depth + 1 or deeper inside any child.

Finding 2 (Low) -- test_the_parent_of_the_node_has_to_match_too asserted a
second positive resolution rather than a rejection, so the suite read as
though it had a negative case for wrong parents when the negative coverage
actually lives in test_a_node_below_the_root_resolves. Renamed to
test_each_uart_returns_its_own_value, and the load-bearing property (decoy is
emitted before soc, so a last-component matcher would return "decoy-uart")
is now stated on the test that depends on it.

Finding 3 (Low) -- a path deeper than FDT_MAX_PATH_DEPTH returned -1, the
same code as a NULL argument, so a caller could not tell a programming error
from malformed input. Now -8, documented in the header's code table.

Verified:
  ctest                                            21/21 PASS
  same under -DEBLDR_SANITIZE=ON (ASan+UBSan)      21/21 PASS
  test_fdt_loader                                  20 tests PASS (was 14)
  test_a_property_on_a_child_is_not_returned_as_the_parents, run against
    this same parser with only the depth guard reverted:
      [FAIL] get_prop_exact(&b, "/soc", "reg", out, &len) != 0
    so it discriminates rather than restating the fix.

Refs #85

* fix(fdt): match node names past the unit address, where real device trees put it

Replaces slash-counting and last-component matching with a component walk
gated on ancestor matches, and matches a component against the node name up
to its unit address -- `/soc/uart` resolves `uart@40011000`, an explicit
`@40011000` still selects exactly that node, and an address not in the tree
does not fall back to a loose match.

Carries the earlier review fixes (depth == target_depth on the property arm;
FDT_MAX_PATH_DEPTH moved to the public header; -8 for an over-deep path,
distinct from -1) and, from the overnight round:

  - the stray `/* Deepest node path ... */` comment left in the .c after the
    define moved to the header is gone (Low)
  - return code -8 is documented in the header's code table again, on this
    branch, because this is the branch where anything returns it -- #84
    dropped the line for exactly that reason

Rebased onto #84's alignment/NOP round; the test-file merge keeps both sides'
suites (garbage-tag, NOP-padding and unaligned-blob from #84; the node-path
and unit-address suites from here).

Verified:
  ctest                                    22/22 PASS
  ctest -DEBLDR_SANITIZE=ON (ASan+UBSan)   22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          24 tests PASS

Refs #85

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Srikanth Patchava <srikanth.patchava@outlook.com>
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 14, 2026
Twenty PRs were merged into master in ninety minutes on 09-07, each on the
base it was written against. Master has not configured, compiled, linked or
passed its tests since. Every nightly and every eos simulation run (which
builds eBoot master) has been red for the same reason.

Configure:
- tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it,
  and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was
  replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111.

Compile (eboot_core):
- embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in
  ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len
  (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c).
- embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c
  and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written.
  CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb).
- embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of
  eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix.
  Header restored (964ebb8).
- embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical
  static scalarbase(); both merged. One copy removed.
- The same pair each added k_low_order[]/messages[] to test_ed25519.c. The
  embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[].

Tests that stopped passing because two merged PRs disagree on behaviour:
- embeddedos-org#104 verifies the image signature at install unconditionally, before the
  anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as
  EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and
  test_fw_transport's XMODEM install can no longer finalize. Both suites now
  stream genuinely signed images. eBoot has no Ed25519 signer in C, so
  tools/gen_fw_update_test_sigs.py signs the exact header prefixes those
  suites build under the RFC 8032 section 7.1 TEST 1 key and emits
  tests/vectors/fw_update_test_sigs.h; the suites serve that key from a
  simulated OTP slot 0. Negative control: one flipped signature byte fails
  test_write_streams_tlv_then_finalize_rejects_below_floor with
  EOS_ERR_SIGNATURE.
- embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the
  image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot()
  returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The
  fixture now places its image in slot A.

Guards from embeddedos-org#95 that later merges walked back, never run until now because the
C configure step failed first:
- embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one.
  Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites
  had no list(APPEND ...) and so no Valgrind run.
- Seven suites assign tests_run = <literal> and their TEST() does not count;
  four suites have no TEST() macro at all. Counted, and classified.
- embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it.

CI plumbing:
- eosim-sanity.yml: the install-validate job is written in bash but ran under
  PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an
  unknown command and `|| { exit 1 }` is an unexecuted script block.
- scorecard.yml: ossf/scorecard-action@v2.4.0 pulls gcr.io, which now demands
  GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green.

Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same
under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest.

Not fixed here, reported separately: core/keystore.c's compiled-in
default_dev_key is described as the RFC 8032 TEST 1 public key but differs
from byte 21 on and is not a point on the curve, so nothing can verify
against it on any board without OTP. With embeddedos-org#104 that makes firmware update
refuse every image on such boards.
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 14, 2026
…harnesses

Answers the second review on embeddedos-org#84.

Finding 2 (Medium) -- eos_fdt_validate() and eos_fdt_get_prop() kept an
out-of-bounds read by design. Both dereferenced hdr->totalsize before any
length was known and handed that attacker-controlled value to the _sized form
as its bound, so calling either on a short buffer read past it before a single
check had run. The header called that a caller warrant; a warrant is not a
check, and it is the exact bug this PR exists to fix. Neither had an in-tree
caller.

Removed rather than documented. There is now one form of each entry point and
it always takes the length:

    int eos_fdt_validate(const void *fdt_blob, uint32_t avail);
    int eos_fdt_get_prop(const void *fdt, uint32_t fdt_len, ...);

An exported unsafe twin in a TCB header is a future boot-path caller
reintroducing the bug with no compiler complaint, which is worth more than the
convenience of a one-argument call. eos_fdt_load() already passed max_size.
The test that pinned the wrapper's behaviour is gone with the wrapper, and
get_prop_sized_exact() collapsed into get_prop_exact() since they became the
same function.

Finding 3 (Medium) -- tests/fuzz/ was built by nothing. EBLDR_BUILD_FUZZ
defaults OFF and no job set it, so the harness added here joined five others
that no CI job compiles. A harness that is never built cannot fail to build,
which is how fuzz_devicetree came to declare a function that did not exist and
sit there unnoticed (eos#50).

Adds a `fuzz-build` job: configure with clang, build every harness, and run
each for five seconds over its own generated inputs. That is not a campaign --
it is enough to catch a harness that no longer compiles or crashes at once,
which is the failure this repo has actually had. Note it needs
EBLDR_BUILD_TESTS=ON as well: tests/fuzz/ is added from tests/CMakeLists.txt,
so EBLDR_BUILD_FUZZ alone configures cleanly and builds no harness at all --
the job would have passed having compiled nothing. Found that locally before
writing the job, not after.

  NOT RUN, and this is the honest limit: this host has no libFuzzer runtime
  (libclang_rt.fuzzer_osx.a is absent from the Xcode toolchain), so the link
  step cannot be reproduced here for any harness, old or new. What I verified
  is that CMake configures with both flags and reports all six targets --
  "Fuzz targets: fuzz_image_verify, fuzz_recovery_protocol, fuzz_fw_update,
  fuzz_crypto, fuzz_bootctl, fuzz_fdt" -- and that fuzz_fdt.c passes
  `cc -fsyntax-only`. The link and the smoke run are CI's to show, and this
  job is what makes them visible.

  Coordination note: embeddedos-org#90 adds a `CI Gate` whose needs list is
  [test, build-arm, static-analysis]. Whichever of embeddedos-org#84 and embeddedos-org#90 lands second
  must add fuzz-build to that list -- and embeddedos-org#90's own
  test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is
  not, which is the guard working rather than a trap.

Finding 1 (High) is a PR-body correction, made there: the diff carries embeddedos-org#94's
master repair because this branch is stacked on it, and the body described
only the FDT parser.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  test_fdt_loader                          13 tests PASS
  grep for a length-free entry point       none remains in the header

Refs embeddedos-org#84
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 15, 2026
Twenty PRs were merged into master in ninety minutes on 09-07, each on the
base it was written against. Master has not configured, compiled, linked or
passed its tests since. Every nightly and every eos simulation run (which
builds eBoot master) has been red for the same reason.

Configure:
- tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it,
  and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was
  replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111.

Compile (eboot_core):
- embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in
  ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len
  (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c).
- embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c
  and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written.
  CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb).
- embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of
  eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix.
  Header restored (964ebb8).
- embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical
  static scalarbase(); both merged. One copy removed.
- The same pair each added k_low_order[]/messages[] to test_ed25519.c. The
  embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[].

Tests that stopped passing because two merged PRs disagree on behaviour:
- embeddedos-org#104 verifies the image signature at install unconditionally, before the
  anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as
  EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and
  test_fw_transport's XMODEM install can no longer finalize. Both suites now
  stream genuinely signed images. eBoot has no Ed25519 signer in C, so
  tools/gen_fw_update_test_sigs.py signs the exact header prefixes those
  suites build under the RFC 8032 section 7.1 TEST 1 key and emits
  tests/vectors/fw_update_test_sigs.h; the suites serve that key from a
  simulated OTP slot 0. Negative control: one flipped signature byte fails
  test_write_streams_tlv_then_finalize_rejects_below_floor with
  EOS_ERR_SIGNATURE.
- embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the
  image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot()
  returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The
  fixture now places its image in slot A.

Guards from embeddedos-org#95 that later merges walked back, never run until now because the
C configure step failed first:
- embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one.
  Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites
  had no list(APPEND ...) and so no Valgrind run.
- Seven suites assign tests_run = <literal> and their TEST() does not count;
  four suites have no TEST() macro at all. Counted, and classified.
- embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it.

CI plumbing:
- eosim-sanity.yml: the install-validate job is written in bash but ran under
  PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an
  unknown command and `|| { exit 1 }` is an unexecuted script block.
- scorecard.yml: ossf/scorecard-action@v2.4.0 pulls gcr.io, which now demands
  GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green.

Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same
under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest.

Not fixed here, reported separately: core/keystore.c's compiled-in
default_dev_key is described as the RFC 8032 TEST 1 public key but differs
from byte 21 on and is not a point on the curve, so nothing can verify
against it on any board without OTP. With embeddedos-org#104 that makes firmware update
refuse every image on such boards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants